-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(babel): keep function names to improve stack traces in tests #189
Conversation
Codecov Report
@@ Coverage Diff @@
## master #189 +/- ##
===========================================
- Coverage 85.61% 64.08% -21.53%
===========================================
Files 17 21 +4
Lines 424 568 +144
Branches 162 214 +52
===========================================
+ Hits 363 364 +1
- Misses 54 165 +111
- Partials 7 39 +32
Continue to review full report at Codecov.
|
I'm having some second thoughts on this. This will only help the case where a project is using Digging through the history a bit, it looks like this plugin came about as part of an UMD feature, which sort of makes sense to me as I'd expect the consumer of the package to deal with the bundling, minifying, treeshaking, etc. when using it from NPM. I'm wondering if should only be added to the config if:
I've also discovered that this issue is killing the display names of React HOCs: const withValue = (value) => (Component) => {
const WithValue = (props) => (
<Component {...props} value={value} />
)
return WithValue
} becomes: const withValue = value => Component => {
return props => /*#__PURE__*/React.createElement(Component, (0, _extends2.default)({}, props, {
value: value
}));
}; (although it could be argued that HOCs should be setting their display name explicitly). |
Yeah, let's only enable the minification plugins when |
src/config/babelrc.js
Outdated
@@ -81,7 +81,7 @@ module.exports = () => ({ | |||
? require.resolve('babel-plugin-transform-inline-environment-variables') | |||
: null, | |||
[require.resolve('@babel/plugin-proposal-class-properties'), {loose: true}], | |||
require.resolve('babel-plugin-minify-dead-code-elimination'), | |||
['babel-plugin-minify-dead-code-elimination', {keepFnName: isTest}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add:
const isMinify = parseEnv('BUILD_MINIFY', false)
Above, and then change this to:
['babel-plugin-minify-dead-code-elimination', {keepFnName: isTest}], | |
isMinify ? require.resolve('babel-plugin-minify-dead-code-elimination') : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also, I didn't even notice I have removed the require.resolve
in my first commit 😅
P.S. I would have taken your suggestion if it were possible to add the parseEnv
at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks 👍
@all-contributors please add @mpeyper for code |
I've put up a pull request to add @mpeyper! 🎉 |
🎉 This PR is included in version 7.5.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Preserve function names when eliminating dead code in tests to enable nicer stack traces when errors occur.
Why:
Fixes #188
How:
I used the
isTest
flag to enable thekeepFnName
option ofbabel-plugin-minify-dead-code-elimination
.Checklist: